Skip to content

array_column: proper support for private properties #4366

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: 2.1.x
Choose a base branch
from
Open

array_column: proper support for private properties #4366

wants to merge 1 commit into from
+67 −1

Conversation

janedbal
Copy link
Contributor

@janedbal janedbal commented Sep 24, 2025 •
edited
Loading

staabm
staabm reviewed Sep 24, 2025
View reviewed changes
src/Type/Php/ArrayColumnHelper.php
$returnTypes[] = $type->getInstanceProperty($propertyName, $scope)->getReadableType();
$property = $type->getInstanceProperty($propertyName, $scope);
if (!$property->isPublic()) {
if (!$type->hasMethod('__isset')->no() && !$type->hasMethod('__get')->no()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe it would make sense to work with ClassReflection->allowsDynamicProperties(). this would also include support for the #[AllowDynamicProperties] attribute etc.

JanTvrdik
JanTvrdik reviewed Sep 24, 2025
View reviewed changes
src/Type/Php/ArrayColumnHelper.php

$returnTypes[] = $type->getInstanceProperty($propertyName, $scope)->getReadableType();
$property = $type->getInstanceProperty($propertyName, $scope);
if (!$property->isPublic()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does getInstanceProperty return non-public property? I though the point of the scope parameter was to filter the property by visibility.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has never worked like that. The main reason why Type::getInstanceProperty() asks for Scope is to correctly transform static type in property type based on the place it's accessed from.

If a class has private property, this method will still return that. It's useful to provide an error like "Access to private property of class X" instead of "Access to undefined property".

Also Scope is used in ClassReflection::getProperty() to decide if it should continute iterating over class reflection extensions, or return the reflection object:

phpstan-src/src/Reflection/ClassReflection.php

Lines 735 to 755 in ac71c94

if ($this->phpClassReflectionExtension->hasProperty($this, $propertyName)) {
$property = $this->phpClassReflectionExtension->getProperty($this, $propertyName, $scope);
if ($scope->canReadProperty($property)) {
return $this->properties[$key] = $property;
}
$this->properties[$key] = $property;
}
if ($this->allowsDynamicProperties()) {
foreach ($this->propertiesClassReflectionExtensions as $extension) {
if (!$extension->hasProperty($this, $propertyName)) {
continue;
}
$property = $this->wrapExtendedProperty($propertyName, $extension->getProperty($this, $propertyName));
if ($scope->canReadProperty($property)) {
return $this->properties[$key] = $property;
}
$this->properties[$key] = $property;
}
}

ondrejmirtes
ondrejmirtes requested changes Sep 25, 2025
View reviewed changes
Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have an idea: What if instead we called Type::toArray() and then asked for $arrayType->getOffsetValueType() with the array_column 2nd arg?

@ondrejmirtes
Copy link
Member

I realize probably not, the returned array there has weird keys. But ObjectType::toArray() still has some useful logic that asks UniversalObjectCratesClassReflectionExtension and is also interested in whether the class is final etc. We should get inspired about that here.

@janedbal
Copy link
Contributor Author

We should get inspired about that here.

Can you be more specific please? Which part of current implementation is wrong and need to be adjusted?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ondrejmirtes ondrejmirtes ondrejmirtes requested changes

+2 more reviewers

@staabm staabm staabm left review comments

@JanTvrdik JanTvrdik JanTvrdik left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

array_column over objects should not extract private properties
4 participants
@janedbal @ondrejmirtes @staabm @JanTvrdik